Use discovered target labels when building samples to send to Stackdriver #15
Conversation
targets/cache.go
Outdated
| Get(ctx context.Context, lset labels.Labels) (*Target, error) | ||
| } | ||
|
|
||
| const DefaultApiEndpoint = "/api/v1/targets" |
There was a problem hiding this comment.
DefaultAPIEndpoint. In Go acronyms are always capitalized.
| logger = log.NewNopLogger() | ||
| } | ||
| return &Cache{ | ||
| logger: logger, |
There was a problem hiding this comment.
Thanks for catching that and the obsolete arg.
targets/cache.go
Outdated
| const DefaultTargetsEndpoint = "/api/v1/targets" | ||
| type Getter interface { | ||
| Get(ctx context.Context, lset labels.Labels) (*Target, error) | ||
| } |
There was a problem hiding this comment.
In cases like this one would rather define an interface at the caller site and not define interfaces along with a single implementation.
That way one can really make use of Go's interfaces being implicit, i.e. the retrieval package won't have to depend on the target package if it takes a TargetGetter as a dependency from main.
Not too relevant in this case yet, but dependency chains tend to explode quickly and that's generally an easy way to cut down on them.
So just a general note, nothing necessary to change right now.
There was a problem hiding this comment.
Thanks for letting me know. I'll move it regardless not to set a bad precedent.
| targetsURL, err := cfg.prometheusURL.Parse(targets.DefaultApiEndpoint) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Maybe create a the entire cache right here and pass that to retrieval.NewPrometheusReader to detangle things a bit?
retrieval/manager.go
Outdated
| sort.Sort(targetLabels) | ||
| m, err := NewMetricFamily(metricFamily, metricResetTimestampMs, targetLabels) | ||
| return m, recordSamples[1:], err | ||
| sort.Sort(output) |
There was a problem hiding this comment.
They are also always sorted in TSDB, including its WAL, so this shouldn't be necessary unless you can spot a place where we violate it again.
retrieval/manager.go
Outdated
| if err != nil { | ||
| return nil, recordSamples[1:], err | ||
| } | ||
| metricLabels := targets.DropTargetLabels(lset, target.DiscoveredLabels) |
There was a problem hiding this comment.
This should take target.Labels since those are the ones actually attached to the metric's label set. Not all target labels will be in discovered labels and relabeling rules make it quite likely the values won't match. So this would be a no-op in most cases.
There was a problem hiding this comment.
Thinking more about this, I wonder though.
The user may end up creating target labels from discovered labels that are necessary to distinguish series. We may not consider the same discovered labels in our resource, possibly mapping two series into the same one.
Hard to say how likely that is, but it's possible.
There was a problem hiding this comment.
As per our discussion offline, changed to drop target.Labels.
Rationale: We don't expect to see the discovered labels verbatim in these labels, and we think in general the target.Labels will be redundant with the Stackdriver MonitoredResource derived from the DiscoveredLabels.
retrieval/manager.go
Outdated
| // Fill in the discovered labels from the Targets API. | ||
| target, err := targetGetter.Get(ctx, lset) | ||
| if err != nil { | ||
| return nil, recordSamples[1:], err |
There was a problem hiding this comment.
We should add a metric at some point tracking this stuff. Can very helpful for remotely debugging user issues.
There was a problem hiding this comment.
Can we do an errors.Wrap here to provide context for the error?
retrieval/manager.go
Outdated
| lset, ok := seriesGetter.get(sample.Ref) | ||
| tsdblset, ok := seriesGetter.get(sample.Ref) | ||
| if !ok { | ||
| return nil, recordSamples[1:], fmt.Errorf("sample=%v", sample) |
There was a problem hiding this comment.
Would be good if this error was more descriptive.
|
Thanks for all the comments. Please take a look. |
| panic(err) | ||
| } | ||
| targetCache := targets.NewCache(logger, nil, targetsURL) | ||
| go targetCache.Run(context.Background()) |
There was a problem hiding this comment.
Nit: this should probably also go into a g.Run call so its properly part of main's lifecycle management. Just killing the goroutine if main exits won't have bad consequences in this case, but those direct go ...s tend to make the main() hard to follow over time.
Here's an example for doing it with contexts. I can push this on top tomorrow.
|
LGTM modulo that one comment. |
Signed-off-by: Fabian Reinartz <freinartz@google.com>
The rationale was buried in a discussion thread in the original PR, and the reasoning isn't obvious until you understand exactly how Target.Labels and MonitoredResource are constructed: #15 (comment)
The rationale was buried in a discussion thread in the original PR, and the reasoning isn't obvious until you understand exactly how Target.Labels and MonitoredResource are constructed: #15 (comment)
3 participants
No description provided.